Skip to content

Fix #11546 FP danglingTemporaryLifetime with unknown member#5256

Merged
chrchr-github merged 6 commits intocppcheck-opensource:mainfrom
chrchr-github:chr_Fix11546
Aug 14, 2023
Merged

Fix #11546 FP danglingTemporaryLifetime with unknown member#5256
chrchr-github merged 6 commits intocppcheck-opensource:mainfrom
chrchr-github:chr_Fix11546

Conversation

@chrchr-github
Copy link
Copy Markdown
Collaborator

No description provided.

Comment thread lib/valueflow.cpp Outdated
ls.byVal(tok, tokenlist, errorLogger, settings);
});
} else if (!isOwningVariables(constructor->nestedIn->varlist)) {
} else if (isOwningVariables(constructor->nestedIn->varlist, args)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lifetimes should not be borrowed when using owning variables.

Comment thread lib/valueflow.cpp
}))
return false;
if (vt->isPrimitive())
if (vt->pointer > 0)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If its a pointer then its not an owning variable.

Comment thread lib/valueflow.cpp
if (vt->pointer > 0)
if (std::none_of(args.begin(), args.end(), [vt](const Token* arg) {
return arg->valueType() && arg->valueType()->type == vt->type;
}))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this check can only be done with pointers. Another user class or container view could still borrow the lifetime.

@chrchr-github
Copy link
Copy Markdown
Collaborator Author

What is isOwningVariables() supposed to mean? In my mind, pointers/references can own a lifetime, but you say no?

@pfultz2
Copy link
Copy Markdown
Contributor

pfultz2 commented Jul 20, 2023

Pointers and references(except shared_ptr and unique_ptr) borrow the lifetime it does not own it.

@chrchr-github
Copy link
Copy Markdown
Collaborator Author

Shouldn't the function then be called hasBorrowingVariables()? Because we do the LifetimeStore stuff when there is at least one ptr/ref.

@pfultz2
Copy link
Copy Markdown
Contributor

pfultz2 commented Jul 29, 2023

Shouldn't the function then be called hasBorrowingVariables()?

Originally, the function was the opposite which is why it was named isOwningVariables(), but you flipped the logic so it does seem hasBorrowingVariables() would be a better name for it.

Comment thread lib/valueflow.cpp Outdated
const ValueType* vt = var.valueType();
if (vt) {
if (vt->pointer > 0)
if (std::none_of(args.begin(), args.end(), [vt](const Token* arg) {
Copy link
Copy Markdown
Contributor

@pfultz2 pfultz2 Jul 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be if (vt->pointer > 0 && std::none_of(args.begin(), args.end(), ...)). I haven't tested on this PR, but it looks like without this check it could lead to FPs in cases like this:

struct A {
    explicit A(int& i) {
        data = std::span<int>{&i, 1};
    }
    void foo() const {
        for(auto& i:data) i++;
    }
    std::span<int> data;
};

int f() {
    int i = -1;
    A a{i};
    i = 0;
    a.foo();
    return 1 / i;
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There already is a FP for this, and if (vt->pointer > 0 ... does not prevent it.

Comment thread lib/valueflow.cpp Outdated
@chrchr-github
Copy link
Copy Markdown
Collaborator Author

Any more feedback?

@chrchr-github chrchr-github merged commit c257c70 into cppcheck-opensource:main Aug 14, 2023
@chrchr-github chrchr-github deleted the chr_Fix11546 branch August 14, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants